Fix: CLOUD-3N2A#898
Conversation
When a persistent PDO connection's transaction state is out of sync with the server (e.g. implicit commit, server-side timeout, connection reset), the defensive rollBack() in startTransaction() would throw "There is no active transaction" and prevent any new transaction from being created. This wraps the cleanup rollback in a try-catch so the failure is silently ignored and a fresh transaction can proceed. Also ensures withTransaction() resets the inTransaction counter immediately when rollbackTransaction() fails during a retry-eligible attempt, preventing the next startTransaction() from incorrectly taking the savepoint branch. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughIn ChangesTransaction State Defensive Fixes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile Summary
Confidence Score: 4/5The transaction recovery changes need attention before merge because rollback failures can be mishandled in paths outside the targeted stale-flag case. The affected code is concentrated in transaction lifecycle handling, and focused runtime checks confirmed key failure modes around retrying after rollback errors and swallowing unexpected PDO rollback exceptions. src/Database/Adapter.php, src/Database/Adapter/SQL.php, and the corresponding Postgres rollback cleanup path need attention.
 T-Rex LogsWhat T-Rex did
Reviews (1): Last reviewed commit: "fix: catch stale-rollback PDOException i..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| try { | ||
| $this->rollbackTransaction(); | ||
| } catch (\Throwable $rollback) { | ||
| $this->inTransaction = 0; |
There was a problem hiding this comment.
This reset also runs when the failing rollback belongs to an inner savepoint. In a nested withTransaction(), a transient error in the inner callback can make the savepoint rollback fail, reset inTransaction to 0, and then retry the inner callback as a new top-level transaction. That retry can issue the top-level cleanup rollback and abort the outer transaction, while the outer callback continues and later commits or no-ops with invalid state. This can commit inner work independently and lose the outer transaction's atomicity.
| $this->inTransaction = 0; | ||
|
|
||
| if ($attempts < $retries) { | ||
| \usleep($sleep * ($attempts + 1)); | ||
| continue; |
There was a problem hiding this comment.
This path resets inTransaction and retries the callback after any rollback exception. For adapters that inherit this base method, a rollback failure can mean the rollback only partially completed rather than that the transaction counter is stale. For example, Memory::rollbackTransaction() replays inverse callbacks; if one inverse throws after changing state, this branch clears the counter and runs the user callback again on dirty state, which can duplicate side effects or leave persisted data inconsistent. Only retry when the rollback failure is the known stale-transaction case, otherwise surface the rollback failure.
| try { | ||
| if ($this->getPDO()->inTransaction()) { | ||
| $this->getPDO()->rollBack(); | ||
| } else { | ||
| $this->getPDO()->prepare('ROLLBACK')->execute(); | ||
| } | ||
| } catch (PDOException) { | ||
| // Defensive cleanup — if there is nothing to roll back | ||
| // (stale persistent connection, implicit commit, etc.) | ||
| // we can safely ignore the failure and proceed. | ||
| } |
There was a problem hiding this comment.
The defensive rollback handling in both src/Database/Adapter/SQL.php and src/Database/Adapter/Postgres.php suppresses every PDOException raised while cleaning up before beginTransaction(). That is safe for the targeted stale “no active transaction” case, but it also hides real cleanup failures such as connection, protocol, or server errors. When that happens, beginTransaction() can run after a failed cleanup attempt and callers can execute on an unknown transaction state with the original rollback error discarded. Please only ignore the specific benign stale-transaction error and rethrow other PDOExceptions.
Artifacts
Repro: focused PHP harness using the real SQL adapter path with a fake PDO call trace
- Contains supporting evidence from the run (text/x-php; charset=utf-8).
Repro: execution output showing rollback error swallowed and beginTransaction called
- Keeps the command output available without making the summary code-heavy.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/unit/TransactionTest.php (1)
59-77: ⚡ Quick winStrengthen retry-branch assertions in this regression test.
This currently proves retry happened, but not that retry took the fresh-transaction path (instead of savepoint flow). Add explicit PDO expectations for
beginTransaction()count and noexec('SAVEPOINT ...')calls.Suggested assertion hardening
- $pdoMock->method('beginTransaction') - ->willReturn(true); + $pdoMock->expects($this->exactly(2)) + ->method('beginTransaction') + ->willReturn(true); + + $pdoMock->expects($this->never()) + ->method('exec');🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/TransactionTest.php` around lines 59 - 77, The test currently verifies that a retry occurred but does not prove the retry took the fresh-transaction path instead of using savepoints. Add an explicit expectation to the pdoMock for the beginTransaction() method to assert it is called exactly twice (the initial transaction attempt plus the retry after rollBack fails). Additionally, add an expectation that the pdoMock's exec() method is never called with arguments containing 'SAVEPOINT' to ensure the savepoint fallback mechanism was not used.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/unit/TransactionTest.php`:
- Around line 59-77: The test currently verifies that a retry occurred but does
not prove the retry took the fresh-transaction path instead of using savepoints.
Add an explicit expectation to the pdoMock for the beginTransaction() method to
assert it is called exactly twice (the initial transaction attempt plus the
retry after rollBack fails). Additionally, add an expectation that the pdoMock's
exec() method is never called with arguments containing 'SAVEPOINT' to ensure
the savepoint fallback mechanism was not used.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b8e2a2a6-b03c-43a1-9bfc-cc6300981bc0
📒 Files selected for processing (4)
src/Database/Adapter.phpsrc/Database/Adapter/Postgres.phpsrc/Database/Adapter/SQL.phptests/unit/TransactionTest.php
1 participant
Resolves CLOUD-3N2A (sentry 7536819395).
PR opened by backfill from pushed branch
fix/start-transaction-stale-rollback.Summary by CodeRabbit